chore(tools): add commentlint and hold the tree to its two rules - #160
Merged
Conversation
The zero-comments rule never bound, because a worker can construct a WHY for any comment and the reviewer is then arguing taste. It passed every case it was applied to while the volume kept climbing. Two rules replace it, both decidable without judgment: a comment may not open with the identifier it documents, and a comment block may not exceed three lines. No golangci-lint rule expresses either one, so tools/commentlint is a single go/ast pass invoked by make lint and by CI's lint job. Rule 1 applies only where Go's doc convention does not reach - unexported declarations, everything in _test.go, and comments inside function bodies - because an exported doc comment is required to open with its identifier and a checker that outlaws idiomatic godoc would be turned off on day one. Directives, generated files and the package doc comment are exempt from both rules. A comment block is a run of adjacent comment lines, and a bare // inside the run does not start a new one. Splitting a long block by blanking a line would otherwise satisfy the check while changing nothing a reader sees. The commits after this one bring the tree to zero, one per directory, because the rules bind only once nothing is grandfathered.
GateRunPRs' and GateStatus' multi-paragraph docs move their per-outcome reasoning onto the branch each paragraph is about, so no claim is dropped to fit three lines.
PaneRead's viewport reasoning splits across the args it explains and the error body it reads, and Status' done paragraph moves onto StatusDone itself.
FindPRByBranch's three paragraphs and writeFakeGHPRListPerRepo's four claims move onto the code each one explains - the target loop, the tier split, the merged-plus-open refusal, the --head argument - so none is dropped to fit three lines.
The long docs on Delete, UnacknowledgedTerminalReport and LastReportedState keep every claim they carried: each one is now a three-line doc plus a comment at the guard or branch the remaining reasoning explains.
migrateSchema's two paragraphs move to the lock and the fresh-database early return they explain, so both survive as three-line blocks at their own code.
Build's and buildClaude's launch reasoning splits onto the code each part is about - the interactive contract on Build, the two env/flag requirements on the args they set, the unverified-harness caveat onto the three builders it applies to - and FirstRunPrompts' zero-value gap moves to the map and the accessor.
tick's identity check, ClassifyUnreachable's dwell, forgetPaneScopedCache's pane trigger, syncTaskState's ordering and lock rule, recordAutoPR's two already-recorded halves and the fakes' herdr/gh fidelity notes each split onto the guard, branch, table entry or call site they explain, so no claim is dropped to fit three lines. The pending-question paragraph that had drifted onto the blink test moves back to the test it describes.
Each claim was stated twice, and in both cases the second copy was the narrower one. cmd/root.go gave the exit-code mapping in ExitError's doc and again on the Code field; internal/harness/harness.go gave the codex/grok/pi fallback in Build's body and again on buildCodex, and the claude/opencode --help provenance on buildCodex and again in Build. The fuller copy stays, at the declaration the claim is about. The root-skip fix this commit originally also carried now sits in the commit that adds the checker, so the checker never ships the bug.
Eight comment lines still named an issue as a bare #N, five of them lines this branch rewrapped. A bare number resolves only for a reader who already knows which repo it belongs to, and does not link at all.
Three comment blocks sat where a reader would not look for them. The herdr PaneRead block explained the --source choice above the return contract instead of above the args line making it; agentsmd's in-body block restated what SPECS.md already owns and left the finding's grading rule undocumented; precondition.go carried a convention for future sentinels inside a slice literal. Two tool facts the herdr block asserted move to internal/faketool/FIDELITY.md, which is where the fake's record of the real tool is read, and the sentinel convention moves to the exit-code bullet in SPECS.md that owns it.
A blank line above a doc comment satisfies the three-line rule without changing anything a reader sees: the same prose still stands in front of the same declaration, and above an exported one godoc drops the first half entirely. The sweep on this branch used the shape at seven sites, so it was a live hole rather than a latent one. The check pairs a doc comment with an undocumented block exactly one blank line above it and reports the two together against the same limit, so a short split is still fine. All seven sites merge back into single three-line blocks, one of them by pointing at the workspace-label record in internal/faketool/FIDELITY.md instead of restating it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Intent
Fix #98: the zero-comments rule did not bind, because it had no checkable form. A reviewer had
to adjudicate whether a given comment carried a real WHY, so the rule kept passing while comment volume climbed.
Replace it with two mechanically enforceable rules and enforce them in the lint step:
The work was deliberately split in two. A lint rule that fails on the existing tree makes CI red, so the checker
and the cleanup had to ship together - but a tree-wide comment sweep conflicts with every branch in flight. So
the checker, its tests and the CONTRIBUTING rewrite came first, then a measured violation count broken down by
file, then a hold until the gate queue drained, and only then the sweep. The count was the deliverable that
decided whether this was one PR or two.
Where the judgement actually was, and what this PR chose:
rule 1 read literally outlaws idiomatic Go. The checker draws its exempt line at exported-ness rather than at
doc-versus-body: exported doc comments outside
_test.goare exempt from rule 1, package docs are exempt fromboth. Getting this wrong makes the checker either useless or unadoptable.
//go:build,//nolint,// #nosec,//go:generateare not prose and areexempt. Generated files are not checked.
//lines are one block.Two counts, because the base moved under this branch. The tree measured 727 violations across 95 files on
post-atqamz/secondhand#45 main. #40 then landed and the checker measures 715 at this
branch's base commit. Both numbers are the same checker; the difference is what the intervening merge added and
removed. The sweep takes it to zero.
The sweep had one instruction that shaped it: much of what #40 added is recorded real-tool
behaviour, and that record is load-bearing - the point of that issue is that a fake which does not match the real
tool hides defects, so deleting the record to satisfy a line limit would undo it. Over-long fidelity records move
into the
internal/faketool/FIDELITY.mdthat already exists and is already referenced, rather than beingtruncated into something that no longer says what the tool does.
What Changed
tools/commentlint: walks a tree, parses each Go file, and reports onefile:line:columnper violation of the two machine-checkable comment rules (a comment may not open with the identifier it documents; a comment block may not exceed three lines), exempting package docs, build/lint directives, generated files, and exported doc comments from rule 1. Wired intomake lintand a newCommentsstep in.github/workflows/ci.yaml.cmd/,internal/, andtests/e2e/to clear the check: identifier-leading openers rewritten and overlong blocks trimmed or moved out, taking the tree from 715 violations at the base commit to zero. The test step's per-file non-comment byte comparison confirms no executable code changed.CONTRIBUTING.mdgains aCommentssection owning the bar for writing a comment at all, the two rules, their exemptions, and why rule 2's occasional wrongness is accepted;AGENTS.mdandSPECS.mdnow point at it and recordtools/commentlintin the directory layout and repo-scaffolding inventories.Risk Assessment
ok: Low: The only new executable logic is a self-contained, unit-tested lint tool wired into make lint and CI; every other edit is comment prose or gofmt realignment, with no assertion, API, schema, or agentsmd generatedBody template change.
Testing
Exercised the linter as a developer would:
go run ./tools/commentlint .is clean (exit 0) on the cleaned-up tree and reports 715 rule-1/rule-2 violations with file:line:col against the pre-cleanup base tree, and a mutation check that hand-reverted two real fixes made it fail at exactly cmd/fields.go:10 and cmd/hold.go:75 before being reverted. Its own unit tests pass under -race,go build ./...and every test binary (including the e2e tag) compile, and the packages with the largest comment churn plus the one fake-script string-literal edit (internal/watcher, internal/agentsmd, cmd) pass under -race, confirming the prose-only edits changed no behavior. I deliberately did not run make lint/golangci-lint/gofmt since the lint phase owns those; this is a CLI tool with no rendered UI, so the reviewer-visible evidence is CLI transcripts rather than screenshots. Everything passed and the worktree is clean.Evidence: Mutation check: tool goes red when two real fixes are reverted
$ git diff --stat # two comment fixes reverted by hand cmd/fields.go | 4 ++-- cmd/hold.go | 7 ++++--- 2 files changed, 6 insertions(+), 5 deletions(-) $ go run ./tools/commentlint . cmd/fields.go:10:1: rule 1: comment opens with the identifier it documents ("pickFields") cmd/hold.go:75:1: rule 2: comment block is 4 lines, the limit is 3 commentlint: 2 violations exit=1Evidence: Before/after: 715 violations at base commit, 0 at target
=== $ commentlint . (target commit, this checkout) === exit=0 === $ commentlint <base-commit-tree> (pre-cleanup code) === cmd/fields.go:10:1: rule 1: comment opens with the identifier it documents ("pickFields") cmd/fleethome_test.go:9:1: rule 2: comment block is 10 lines, the limit is 3 cmd/launch.go:35:1: rule 2: comment block is 21 lines, the limit is 3 ... (30 shown of many) --- total violations at base --- commentlint: 715 violations exit=1Evidence: Exact CI/make lint step on the target tree
$ go run ./tools/commentlint . # the exact step CI and make lint run exit=0Pipeline
Updates from git push no-mistakes
ok: **intent** - passed
ok: No issues found.
ok: **Rebase** - passed
ok: No issues found.
warning: **Review** - 3 infos
internal/herdr/client.go:310- The rule-2 rewrite parked several overflow comment blocks below the line they explain, so they now read as documenting the wrong statement. internal/herdr/client.go:310 explains the--source recentargument of theargs := []string{...}line above it but sits abovec.run(args...). Same pattern: cmd/precondition.go:15 (guidance for adding a new sentinel now reads as documentingstate.ErrTaskNotFound), internal/agentsmd/agentsmd.go:188 ("It never writes" describes Check but sits above thehome.IsHomecall), internal/watcher/watcher.go:478 ("Deliberately not a Pending Decision" sits abovekind, outcome := ...). Non-functional; moving each block back above its subject (or into SPECS.md per CONTRIBUTING) restores the referent.internal/watcher/watcher_test.go:177- The same pass requalified many issue references toatqamz/secondhand#Nbut left bare#Non five rewritten comment lines: internal/agentsmd/agentsmd_test.go:188 (#87's fix), internal/completion/completion_test.go:107 (brief for #61), internal/watcher/watcher_test.go:177 (#30/#32/#33), internal/watcher/watcher_test.go:2015 (Covers #85), internal/watcher/watcher_test.go:2410 (Covers #81's hard part). Inconsistent with the rest of the branch; qualify them the same way.CONTRIBUTING.md:1- Rule 2 is satisfiable by inserting a blank line between two comment blocks, and this change uses that escape repeatedly (tests/e2e/fakes_test.go realBinsOnPath, cmd/teardown.go checkLandedWork, internal/watcher/events.go). CONTRIBUTING closes the bare-//loophole but not this one. Applied above an exported declaration it would silently drop the first block from godoc, and commentlint cannot detect it because only the adjacent group is the Doc. Verified zero live instances today (no exported declaration is preceded by a split block), so this is a latent hazard rather than a current defect - worth a note in CONTRIBUTING's Comments section or a follow-up check in the linter.ok: **Test** - passed
ok: No issues found.
nix develop --command go build ./...nix develop --command go test -race -count=1 ./tools/commentlint/...nix develop --command go test -race -count=1 ./internal/watcher/... ./internal/agentsmd/... ./cmd/...go test -run "^$" ./...andgo test -tags=e2e -run "^$" ./tests/e2e/...(compile every test binary, including the e2e build tag)go run ./tools/commentlint .on the target tree - the exact command in .github/workflows/ci.yaml and the Makefile lint target (exit 0, no output)commentlint <base-tree>against agit archiveof 46415fbc511b5f29c405a6eaf5909eae77483ddb extracted to the evidence dir (715 violations, exit 1)Mutation check: hand-reverted the rule-1 fix in cmd/fields.go and the rule-2 fix in cmd/hold.go, re-rango run ./tools/commentlint .(flagged cmd/fields.go:10 and cmd/hold.go:75, exit 1), thengit checkout --both files and confirmedgit status --porcelaincleanPer-file non-comment byte comparison of every changed .go file between base and target to confirm the cleanup is comment-onlyfix: **Document** - 1 issue found -> auto-fixed ok:
SPECS.md:2761- SPECS.md's "Repo scaffolding" section enumerates every tracked non-generated scaffolding file (Makefile, .golangci.yaml, .gitignore, flake.nix, workflows) and has no entry for the new trackedtools/commentlint, and the "Directory layout" tree at SPECS.md:117 lists top-level tracked paths and internal packages withouttools/. Left unedited on purpose: both lists already predate this change in their drift (the tree also omitstests/,Makefile,SPECS.md,CONTRIBUTING.md,.github/, and theghutil,selfupdate,faketoolandage-adjacent packages), CONTRIBUTING.md's "Comments" section is the declared owner of the comment rules, and adding one line here would be a third prose copy while a full reconciliation is a separate documentation pass. Follow-up: reconcile those two inventories against the tracked tree once, or reduce them to pointers.fix: Fix: record tools/commentlint in SPECS.md scaffolding inventories
ok: Re-checked - no issues remain.
ok: **Lint** - passed
ok: No issues found.
ok: **Push** - passed
ok: No issues found.
Closes #98